-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Annotorious upgrade, delete key fix (#1221), polygon tool (#1311) #1456
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1456 +/- ##
===========================================
+ Coverage 98.69% 98.77% +0.08%
===========================================
Files 205 205
Lines 11399 12272 +873
===========================================
+ Hits 11250 12122 +872
- Misses 149 150 +1 |
<button class="primary popout-close-button" type="button" aria-label="Close"><i class="ph-x"></i></button> | ||
<button class="popout-button" type="button"> | ||
<svg> | ||
<use xlink:href="{% static 'img/ui/desktop/all/pop-out.svg' %}#pop-out-icon" /> | ||
</svg> | ||
<span class="sr-only">Pop out</span> | ||
</button> | ||
<fieldset class="toolbar"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these added to the geniza template instead of to tahqiq?
I don't object necessarily, just want to understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't thought to do that! Was conceptualizing it as a Geniza feature so put it in Geniza, but I think it makes more sense to pass the toolbar element (in this case the fieldset) as a param to tahqiq, and instantiate the button elements there.
{# rectangle tool #} | ||
<label class="rect-tool active-tool"> | ||
<input type="radio" checked /> | ||
<span class="sr-only">Rectangle tool</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how meaningful "rectangle tool" is; how about rephrasing to describe better what these do?
<span class="sr-only">Rectangle tool</span> | |
<span class="sr-only">Select a rectangle</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will put this suggestion into the tahqiq update, thanks!
const anno = Annotorious(viewer, { disableDeleteKey: true }); | ||
|
||
// get toolbar element | ||
const toolbarContainer = this.element.querySelector(".tahqiq-toolbar"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you still need to bind this here? what does the annotation controller need to do with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The annotation controller needs to pass it to tahqiq instantiation, but doesn't need to define a variable for it so I've simplified.
In this PR
Per #1221:
disableDeleteKey
option in Annotorious to fix a bug with editing annotation labelsPer #1311: